Fix header fragmentation parsing (Fix #21)#27
Fix header fragmentation parsing (Fix #21)#271st1 merged 3 commits intoMagicStack:masterfrom youknowone:framented-broken
Conversation
| p = httptools.HttpRequestParser(m) | ||
|
|
||
| REQUEST = ( | ||
| b'PUT / HTTP/1.1\r\nHost: localhost:1234\r\nContent-', |
There was a problem hiding this comment.
I slightly changed the fragmented point and it now starts to be broken
There was a problem hiding this comment.
I think it was on purpose where the splitting was, especially the empty HTTP header.
There was a problem hiding this comment.
Another test which feeds byte-for-byte would also be nice.
There was a problem hiding this comment.
Thanks for the advice. I added a few more tests
|
|
||
| cdef _on_headers_complete(self): | ||
| self._maybe_call_on_header() | ||
| if self._current_header_name is not None: |
There was a problem hiding this comment.
Not sure why you moved this check out of _maybe_call_on_header (so it's now duplicated).
Seems to me you could keep it in there, but just move the reset of _current_XXX out of if, something like:
def _maybe_call_on_header():
if self._current_header_name is not None:
# do the real call
# do the resetThoughts?
| cdef _on_header_field(self, bytes field): | ||
| self._maybe_call_on_header() | ||
| self._current_header_name = field | ||
| if self._current_header_name is None: |
There was a problem hiding this comment.
Not sure about the if/elif/else here.
Why not just calling _maybe_call_on_header if _current_header_name is None (which means we are starting a new header) ?
| cdef _on_header_field(self, bytes field): | ||
| self._maybe_call_on_header() | ||
| self._current_header_name = field | ||
| if (self._current_header_value is not None or |
There was a problem hiding this comment.
I still don't get why we need to check _current_header_value here, given it's reset at each call to _maybe_call_on_header, but I'm certainly missing something :)
Seems to me this cover a case where both _current_header_name and _current_header_value would be set, but I fail to understand how this could happen, given they are reset all together when calling _maybe_call_on_header and any call to _on_header_field with an empty header name would call _maybe_call_on_header, and thus reset _current_header_value.
Can you elaborate a bit? :)
There was a problem hiding this comment.
One header field can trigger multiple _on_header_field call. (the else part)
I think it must be there unless you know better method to check it without _current_header_value.
There was a problem hiding this comment.
Unless I'm missing something, I understand that _on_header_field will be called (once or more) for the header name only, so it seems to me that the check on _current_header_value is useless.
Is there a test that fails if you remove it?
There was a problem hiding this comment.
I think that's the only part I fixed for the problem.
Yes, than the tests I added fail.
I am checking weather it is same header name or not by checking the header value. Once the value is fed, it must be a new header name.
There was a problem hiding this comment.
OK, now I think I get it :)
Here is the fix I suggest (from master):
diff --git a/httptools/parser/parser.pyx b/httptools/parser/parser.pyx
index 064e55c..b9b7175 100644
--- a/httptools/parser/parser.pyx
+++ b/httptools/parser/parser.pyx
@@ -95,10 +95,9 @@ cdef class HttpParser:
self._last_error = None
cdef _maybe_call_on_header(self):
- if self._current_header_name is not None:
+ if self._current_header_value is not None:
current_header_name = self._current_header_name
current_header_value = self._current_header_value
-
self._current_header_name = self._current_header_value = None
if self._proto_on_header is not None:
@@ -107,7 +106,10 @@ cdef class HttpParser:
cdef _on_header_field(self, bytes field):
self._maybe_call_on_header()
- self._current_header_name = field
+ if self._current_header_name is None:
+ self._current_header_name = field
+ else:
+ self._current_header_name += fieldYou tests should still pass :)
There was a problem hiding this comment.
Have you tested it?
There was a problem hiding this comment.
I actually like the fix @yohanboniface proposes. Can we use it in this PR?
There was a problem hiding this comment.
I am sorry. I totally forgot about this PR due to other schedules. Let me try it again.
|
I replaced the fix to @yohanboniface's work |
|
Merged. Guys, a wholehearted thank you for working on this PR! |
|
I've just released httptools 0.0.10 with this fix. |
|
Thanks! |
No description provided.